-
Notifications
You must be signed in to change notification settings - Fork 130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Sunset QLC requirements from cache behaviour #629
Sunset QLC requirements from cache behaviour #629
Conversation
Thanks! Btw, I have a local branch with guild cache and member cache changes, so we don't duplicate any work :-) |
Btw, I was looking at the message cache QLCs the other day and I think we can replace it with match specs |
We are going to win! |
You might be right, match specs or in the case of message eviction using I'll see what I can come up with for this PR |
Oddly enough, I think we should keep most of them on the Mnesia Message cache for performance reasons. For example: iex 1> handle = :nostrum_message_cache_qlc.by_channel_and_author(12345, 5678, 900000, :infinity, Nostrum.Cache.MessageCache.Mnesia)
iex 2> IO.puts(:qlc.info(handle)) ets:match_spec_run(lists:flatmap(fun(V) ->
mnesia:index_read(nostrum_messages,
V, 3)
end,
[12345]),
ets:match_spec_compile([{{'_', '$1', '$2', '$3',
'$4'},
[{'andalso',
{'=<', '$1',
{const, infinity}},
{'andalso',
{'=:=', '$3',
{const, 5678}},
{'andalso',
{'=:=', '$2',
{const, 12345}},
{'>=', '$1',
{const, 900000}}}}}],
['$4']}])) QLC will be able to leverage indexes, however plain selects don't seem to be able to. |
The only one that seemed to make sense to drop QLC usage on in my opinion was how eviction was done if the type was The rest would be leveraging indexes, which are generally better performant |
4d1350f
to
c5a2989
Compare
Maybe long term we consider adding Ecto as an optional dependency and provide our own Ecto Repo backed cache implementations |
This is ready, but I think I'll need to rebase on #630 after its merged before test will actually pass |
I thought about this too, I looked at how Adam does it in Coxir and it seems smart, but it's a ton of work and to be honest I'm not a huge fan of locking us into Elixir-only libraries for some reason. If we could do it incrementally I'd be interested but currently I'm not sure how many more cache layer rewrites I have in me 😂 |
Let me try to rephrase the above a little. Similar to how we only compile the Mnesia caches when Mnesia is available, we only compile the Ecto backed ones when the user has Ecto installed and then provide Ecto.SQL migrations that also are only compiled when Ecto.SQL is installed. No cache rewriting, just another type beyond Mnesia that users can opt-into using instead of the defaults. I don't think that's locking us into an Elixir-only library as they'd remain optional dependencies? |
Oh, I see what you mean, but what I was getting at is that from my
understanding we would need to provide our actual library types (e.g.
all structs, or at least those that are cacheable) as Ecto definitions,
wouldn't we?
Coxir has a really smart design here. Perhaps 2024 is the year of the
nostrum + coxir fusion.
Some say that Adam's dog is taller than Dwayne "The Rock" Johnson,
and that he sleeps with one eye open.
|
I've merged my PR, there are some formatter problems that I'm not really
sure where they stem from. I think it's some incomptibility between the
formatter version used on my local environment and the one we use in the
test matrix.
All that to say, when you rebase from master, please run `mix format` :)
|
d2f746b
to
224b88c
Compare
There were some minor errors that needed to be fixed after rebasing, so please do review carefully.
I was thinking something along the lines of how the mnesia caches operated on fields that we didn't have indexes on. We define a custom Ecto.Type that just calls Or maybe we dump them as JSON and cast them back when loading, meaning that we just have to make sure all our types impl Jason.Encoder 🤔 |
I was thinking something along the lines of how the mnesia caches operated on fields that we didn't have indexes on. We define a custom Ecto.Type that just calls `:erlang.term_to_binary()` on the struct before writing in.
Or maybe we dump them as JSON and cast them back when loading, meaning that we just have to make sure all our types impl Jason.Encoder 🤔
I have to be honest in saying that I don't like either of these
solutions :-)
I think for now our caches should be good - I hope the performance
problems are fixed now, and if not we'll keep hacking away at it, even
if we have to write the cache layers in C, we will have the best cache
on the planet. I know this because I wrote it, and Joe wrote it, and we
all kind of wrote it, I think what nostrum offers here is simply ahead
of the class, ahead of the curve. Cache backends are a brilliant idea,
next up I have been thinking of a concept to allow you to run nostrum
across multiple hosts in the same rack (meaning assuming a stable
network connection), I will release this and I will release that and I
think that at the end of the day nostrum will shine bright in the sun
like the Roman Empire.
|
Thanks! |
See #625 for discussion around why, but the TL;DR is that was decided that while QLC is a cool tech we've had far too many issues with it.
As such we're removing it as a requirement for cache modules to implement, however some cache modules may choose to keep it where it makes sense to do so. Such as the message cache for operations that would require a full table scan anyway.
Caches updated:
guild cachemember cache